Support basePath for relative href prop #248
Conversation
🦋 Changeset detectedLatest commit: 4c10b28 The changes in this PR will be included in the next version bump. This PR includes changesets to release 1 package
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
upreeti
changed the title
| @@ -72,8 +72,12 @@ const Link = forwardRef<HTMLButtonElement | HTMLAnchorElement, LinkProps>( | |||
| )) || | |||
| '' | |||
| : to; | |||
| const IS_ABSOLUTE_LINK_REGEX = /^((?:(http|https):\/\/)|\/\/)/; | |||
There was a problem hiding this comment.
The code change itself looks good. However, I wonder if href should be prefixed.
For to, it makes sense because to must be one of routes.
But for href, it's basically any URL. I know that you have the check whether href is a relative URL. If yes, then prefix. But should we prefix it at the product code?
const { getBasePath } = useRouterActions();
const basePath = getBasePath();
const href = `${getBasePath}/something`;
<Link href={href} />
There was a problem hiding this comment.
Is it too many places we need to prefix if it's done at the product level?
There was a problem hiding this comment.
yes, that's why we wanted to have it built into the code here since it would be more scalable and if we did have it at the product level there would be too many places to prefix. We are also using the MenuLinkItem component (which uses Link downstream), and it doesn't take a to prop (only href). Also, it seems more intuitive that once the basePath prop is passed in, it will apply to both the to prop and href as opposed to having to define it for href again
we have this change as a patch fix right now, but I thought it might be useful to upstream
We are using the
basePathprop for navigation in our application and for our side navigation we are using the MenuLinkItem which takes anhrefprop. Because of this, we did not see thebasePathbeing added to the route when hovering over the side navigation.I found this PR which prefixes basePath when using the
toprop, and I was wondering if this could be extended to relative hrefs as well. We added a patch fix for this in our code, but I created this PR to upstream this change of prefixing to relativehrefpaths, after reaching out to @pancaspe87